Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨Source Youtube Analytics - Migrate Python CDK to Low-code CDK to Manifest-only #42838

Open
wants to merge 40 commits into
base: master
Choose a base branch
from

Conversation

topefolorunso
Copy link
Collaborator

@topefolorunso topefolorunso commented Jul 27, 2024

What

Migrate Youtube Analytics Python CDK to Low-code CDK to Manifest-only fixes https://github.com/airbytehq/airbyte-internal-issues/issues/8865

How

Developed using UI Builder and the Low-code CDK

Recommended reading order

  1. manifest.yaml

🚨 User Impact 🚨

Are there any breaking changes? What is the end result perceived by the user?

For connector PRs, use this section to explain which type of semantic versioning bump occurs as a result of the changes. Refer to our Semantic Versioning for Connectors guidelines for more information. Breaking changes to connectors must be documented by an Airbyte engineer (PR author, or reviewer for community PRs) by using the Breaking Change Release Playbook.

If there are breaking changes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.

Pre-merge Actions

Expand the relevant checklist and delete the others.

Updating a connector

Community member or Airbyter

  • Grant edit access to maintainers (instructions)
  • Unit & integration tests added

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

  • Create a non-forked branch based on this PR and test the below items on it
  • Build is successful
  • If new credentials are required for use in CI, add them to GSM. Instructions.

Copy link

vercel bot commented Jul 27, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 26, 2025 2:03pm

@octavia-squidington-iii octavia-squidington-iii added area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/youtube-analytics labels Jul 27, 2024
@natikgadzhi
Copy link
Contributor

natikgadzhi commented Aug 8, 2024

/format-fix

Format-fix job started... Check job output.

✅ Changes applied successfully. (b0b5405)

@topefolorunso
Copy link
Collaborator Author

@natikgadzhi Can you confirm the test credentials are valid. ps: I passed the check command in local

image

@natikgadzhi
Copy link
Contributor

Ummm the screenshot is failing check.

I'll run regression tests once basic CATs pass

@topefolorunso
Copy link
Collaborator Author

I think it's an issue with the test credentials. I tested with mine and check passed

@natikgadzhi
Copy link
Contributor

Ok we sort of have a youtube channel, let me try and update the creds.

@natikgadzhi
Copy link
Contributor

Kicked-off a pre-release build, will try it out and report back. Didn't have time yet.

@natikgadzhi
Copy link
Contributor

natikgadzhi commented Aug 21, 2024

With the pre-release build, on my personal account on cloud, I get a check error:

'Cannot attempt to connect to stream channel_annotations_a1 - no stream slices were found, likely because the parent stream is empty.'

It maybe because of the lack of data in my channel, but let me try the current version to verify.

UPD: the current version fails with The Youtube account is not valid. Please make sure you're trying to use the active Youtube Account connected to your Google Account. — my test account is not a very good fit, huh.

@DanyloGL can you help test this to get this over the finish line?

@topefolorunso
Copy link
Collaborator Author

topefolorunso commented Jan 24, 2025

/format-fix

Format-fix job started... Check job output.

✅ Changes applied successfully. (31e535a)

@topefolorunso
Copy link
Collaborator Author

topefolorunso commented Jan 24, 2025

/format-fix

Format-fix job started... Check job output.

✅ Changes applied successfully. (210ee70)

@topefolorunso
Copy link
Collaborator Author

topefolorunso commented Jan 25, 2025

/format-fix

Format-fix job started... Check job output.

✅ Changes applied successfully. (223ffd2)

@topefolorunso
Copy link
Collaborator Author

@natikgadzhi I need help here implementing the async retriever please

@natikgadzhi
Copy link
Contributor

Converting this would indeed be a very big win.

@lazebnyi or @darynaishchenko, would any of you find time this week to take a good look into this and record a loom + provide examples to @topefolorunso on how to impolement async retrievers? Getting another big connector to manifest-only would be lovely.

Copy link
Collaborator

@lazebnyi lazebnyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to implement AsyncRetriever and an error handler in the manifest. Everything else looks good.



@dataclass
class YoutubeAnalyticsErrorHandler(DefaultErrorHandler):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

      error_handler:
        type: DefaultErrorHandler
        backoff_strategies:
          - type: ConstantBackoff
             backoff_time_in_seconds: 30
        response_filters:
                type: HttpResponseFilter
                      action: FAIL
                      http_codes:
                        - 429
                      predicate: >-
                        {{response.get("error", {}).get("details", [])[0].get("reason")  == "RATE_LIMIT_EXCEEDED" and   response.get("error", {}).get("details", [])[0].get("metadata", {}).get("quota_limit") == "FreeQuotaRequestsPerDayPerProject"}}
                      error_message: >-
                        {{ "Exceeded daily quota: " + response.get("error", {}).get("details", [])[0].get('metadata', {}).get('quota_limit_value')} + " reqs/day" }}

It is better to describe error handler in the manifest instead have a custom component.



@dataclass
class CreationRequester(HttpRequester):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this behavior you could use AsyncRetirever to avoid custom component.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried implementing the async retriever here @lazebnyi . This custom component is only a component of the async retriever to the best of my understanding. I will watch the loom and be back with questions 🤞🏼

https://github.com/airbytehq/airbyte/pull/42838/files#diff-cb52a8ae8fa55be3587d13879abff80da4c14577466259c5592a2701f3c84735R428-R469

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/youtube-analytics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants